Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow migrations with asymmetric tables #5603

Merged
merged 7 commits into from
Jun 28, 2022

Conversation

danieltabacaru
Copy link
Collaborator

@danieltabacaru danieltabacaru commented Jun 17, 2022

What, How & Why?

This PR fixes and improves several things around asymmetric sync.

  • The bug in RCORE-1120 resulted as an implementation oversight due to using boolean values for asymmetric and embedded properties in ObjectSchema. This is now replaced by an Enum value.
  • Migration tests were added to make sure it is not allowed to migrate to or from an asymmetric table.
  • Asymmetric objects can now contain lists and dictionaries of embedded objects.
  • Integration test was added to validate an asymmetric table can be added in development mode.

Fixes RCORE-1120.

(Commits are logically separated to make review easier)

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Jun 17, 2022
@danieltabacaru danieltabacaru requested a review from jbreams June 17, 2022 13:33
@jbreams
Copy link
Contributor

jbreams commented Jun 17, 2022

Can you rebase this onto latest master to fix all the object-store-tests?

@danieltabacaru danieltabacaru force-pushed the dt/disallow_migrations_with_asymmetric_tables branch from 02a0584 to 774161d Compare June 17, 2022 14:19
@@ -423,9 +423,12 @@ static inline realm_class_info_t to_capi(const ObjectSchema& o)
info.num_properties = o.persisted_properties.size();
info.num_computed_properties = o.computed_properties.size();
info.key = o.table_key.value;
if (o.is_embedded) {
if (o.table_type == ObjectSchema::TableType::Embedded) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this just be a switch statement?

o << "TopLevelAsymmetric";
break;
default:
o << "Unknown table type";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this isn't just going to be REALM_UNREACHABLE, do we want to output the value as an integer here for debugging purposes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

using IsAsymmetric = util::TaggedBool<class IsAsymmetricTag>;
/// The type of tables supported by a realm.
/// Note: Enumeration value assignments must be kept in sync with <realm/table.hpp>.
enum class TableType : uint8_t { TopLevel = 0, Embedded = 0x1, TopLevelAsymmetric = 0x2 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a static_assert in the implementation file that verifies the values match? Is the reason we don't want to just use the actual Table::Type enum here that we don't want to pull in table.hpp as an include in this file?

also, from a naming perspective. should this really be ObjectType rather than TableType at the ObjectStore layer?

Copy link
Collaborator Author

@danieltabacaru danieltabacaru Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on the name (as a fact I wasn't happy with it), but I think ObjectType makes more sense.

The reason is that since some SDKs interface with OS directly, I find it a bit weird that they would need to know about table.hpp and Table::Type when creating schemas (I want everything doing with schemas to be contained in object_schema.hpp).

@danieltabacaru danieltabacaru force-pushed the dt/disallow_migrations_with_asymmetric_tables branch from b01d99a to f05db09 Compare June 28, 2022 08:32
@danieltabacaru danieltabacaru merged commit 92aca0b into master Jun 28, 2022
@danieltabacaru danieltabacaru deleted the dt/disallow_migrations_with_asymmetric_tables branch June 28, 2022 09:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants